Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add coingecko #53

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add coingecko #53

wants to merge 2 commits into from

Conversation

ryonzhang
Copy link
Contributor

No description provided.

Copy link
Collaborator

@kahkeng kahkeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just some early comments

}
}
"""
def getGlobalDefiData():
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be good to use the pythonic style for functions here, just for consistency

'symbol': 'Puuvilla'}]
"""
def get_top_nft_by_24h_native_token():
url = f'https://api.coingecko.com/api/v3/nfts/list?order=h24_volume_native_desc'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe good to extract the base api url out into a constant?

@@ -141,6 +141,50 @@ Required parameters:
Return value description:
-JSON object with trait names and values of the NFT asset
---
Widget magic command: <|fetch-nft-list-order-by-trading-volume-native-token()|>
Description of widget: List all supported NFT, ordered by 24h trading volume of native token
Connected wallet required: no
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry i experimented with removing this in another diff to simplify wallet stuff, so we can drop this. but we should add a "Return value description" to any fetch-* commands, so the AI knows what to expect.

Also, this diff adds a lot more commands, I think the description of the widget needs to be very clear about what it is doing, so the tool knows when to pull up a given widget and use it. E.g. instead of market cap, it could state market capitalization. also trading volume of what tokens, etc.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, this might cause performance of other demo flows to drop, we will need to test it rigorously, or collapse into fewer, more specific use cases. not sure

response = requests.get(url)
response.raise_for_status()
result = response.json()['companies']
return ','.join(["{name} with total holding of {total_holdings}".format(name=item['name'],total_holdings=item['total_holdings']) for item in result])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should these return comma-delimited text lists? or should we try to use table/list containers?

it would be helpful to have the return describe something like "An NFT collection with name {name} and address {address}", instead of just "{name} with address {address}". If you see the center.py integration, there could be some useful examples to follow for containers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants